feat! Break out LSM module from SDK #3455
Conversation
|
Progress is a beautiful thing |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Eric-Warehime
changed the title
This requires migration that sets the
This will fix the first issue (as the
Is there a plan to reintroduce this security feature? |
IIUC, this means that the liquid staking caps will cover only tokenized shares issued by the LSM module and no longer include delegation from liquid staking providers, right? This means that another key security feature of LSM is removed, i.e.,
Are there plans to reintroduce this security feature? |
|
nit:
This functionality is per delegator. |
x/lsm/keeper/liquid_stake.go
Outdated
|
|
||
| // Unlocks all queued tokenize share authorizations that have matured | ||
| // (i.e. have waited the full unbonding period) | ||
| func (k Keeper) RemoveExpiredTokenizeShareLocks(ctx context.Context, blockTime time.Time) ([]string, error) { |
There was a problem hiding this comment.
This method is never called. In the original implementation it was called in the BeginBlock of the staking module.
x/lsm/keeper/liquid_stake.go
Outdated
| // The totals are determined by looping each delegation record and summing the stake | ||
| // if the delegator is the lsm. | ||
| // This function must be called in the upgrade handler which onboards LSM | ||
| func (k Keeper) RefreshTotalLiquidStaked(ctx context.Context) error { |
There was a problem hiding this comment.
This method is never called. Should assess if the logic is needed for migration. Otherwise, remove it to simplify code.
x/lsm/keeper/liquid_stake.go
Outdated
| // SafelyDecreaseValidatorBond decrements the validator's self bond | ||
| // so long as it will not cause the current delegations to exceed the threshold | ||
| // set by validator bond factor | ||
| func (k Keeper) SafelyDecreaseValidatorBond(ctx context.Context, valAddress sdk.ValAddress, shares math.LegacyDec) error { |
There was a problem hiding this comment.
This method is not called. If the validator bond concept was removed, there is no reason to keep this method.
x/lsm/keeper/liquid_stake.go
Outdated
|
|
||
| // Increase validator bond shares increments the validator's self bond | ||
| // in the event that the delegation amount on a validator bond delegation is increased | ||
| func (k Keeper) IncreaseValidatorBondShares(ctx context.Context, valAddress sdk.ValAddress, shares math.LegacyDec) error { |
There was a problem hiding this comment.
Not sure I understand the logic behind this function given that the validator bond was removed. Currently, it is called by RedeemTokensForShares when the delegator address matches the validator address, but I'm not sure of the reason for doing so. What is the invariant for validator.ValidatorBondShares?
x/lsm/keeper/liquid_stake.go
Outdated
| } | ||
|
|
||
| // DecreaseValidatorLiquidShares decrements the liquid shares on a validator | ||
| func (k Keeper) DecreaseValidatorLiquidShares(ctx context.Context, valAddress sdk.ValAddress, |
There was a problem hiding this comment.
In the original implementation, this method is called in four scenarios:
- a redelegation
- an undelegation
- converting tokenized shares back to native delegations (RedeemTokensForShares)
- slashing a redelegation
Now, the method is called only from RedeemTokensForShares. What is the reason for the change in behavior?
x/lsm/keeper/liquid_stake.go
Outdated
| // | ||
| // The percentage of liquid staked tokens must be less than the GlobalLiquidStakingCap: | ||
| // (TotalLiquidStakedTokens / TotalStakedTokens) <= GlobalLiquidStakingCap | ||
| func (k Keeper) SafelyIncreaseTotalLiquidStakedTokens(ctx context.Context, amount math.Int, sharesAlreadyBonded bool) error { |
There was a problem hiding this comment.
This method is only called when tokenizing shares (as compared to the original implementation). Is this because delegations from LSPs are no longer considered to be liquid staked tokens? If so, what's the reason behind keeping track of this value? Is there a reason to cap the amount of tokenized shares?
x/lsm/keeper/hooks.go
Outdated
| initialLiquidTokens := validator.TokensFromShares(liquidVal.LiquidShares).TruncateInt() | ||
| slashedLiquidTokens := fraction.Mul(sdkmath.LegacyNewDecFromInt(initialLiquidTokens)) | ||
|
|
||
| decrease := slashedLiquidTokens.TruncateInt() |
There was a problem hiding this comment.
The way the decrease value is calculated here is different from the original implementation. Would be useful to have a test verifying that the calculation is correct.
x/lsm/keeper/msg_server.go
Outdated
| // If this tokenization is NOT from a liquid staking provider, | ||
| // confirm it does not exceed the global and validator liquid staking cap | ||
| // If the tokenization is from a liquid staking provider, | ||
| // the shares are already considered liquid and there's no need to increment the totals | ||
| if !k.DelegatorIsLiquidStaker(delegatorAddress) { |
There was a problem hiding this comment.
This is no longer accurate given that the caps no longer include delegations from LSPs.
x/lsm/keeper/msg_server.go
Outdated
| // If this redemption is NOT from a liquid staking provider, decrement the total liquid staked | ||
| // If the redemption was from a liquid staking provider, the shares are still considered | ||
| // liquid, even in their non-tokenized form (since they are owned by a liquid staking provider) | ||
| if !k.DelegatorIsLiquidStaker(delegatorAddress) { |
There was a problem hiding this comment.
ditto: This is no longer accurate given that the caps no longer include delegations from LSPs.
x/lsm/keeper/msg_server.go
Outdated
| return nil, types.ErrTokenizeSharesDisabledForAccount.Wrapf("tokenization will be allowed at %s", unlockTime) | ||
| } | ||
|
|
||
| // ValidatorBond delegation is not allowed for tokenize share |
| // BeginBlocker removes expired tokenize share locks | ||
| func (k *Keeper) BeginBlocker(ctx context.Context) error { | ||
| sdkCtx := sdk.UnwrapSDKContext(ctx) | ||
| _, err := k.RemoveExpiredTokenizeShareLocks(ctx, sdkCtx.BlockTime()) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| key := types.GetTokenizeSharesLockKey(address) | ||
| err := store.Delete(key) | ||
| if err != nil { | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| for _, k := range keys { | ||
| err := store.Delete(k) | ||
| if err != nil { | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
|
|
||
| // BeginBlock returns the Begin Blocker for the liquid module. | ||
| func (am AppModule) BeginBlock(ctx context.Context) error { | ||
| return am.keeper.BeginBlocker(ctx) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
Overview
This pull request moves the liquid staking functionality from the SDK fork into its own dedicated module,
x/lsm.The following is a summary of the APIs and types of this module and highlights some of the key differences between
this implementation and the previously integrated functionality living in the SDK's
features/v0.50.x-lsmbranch.You can refer to this diff to see the full changeset which the fork requires--it can be useful to refer to this and compare to the diff in this
PR, although I've tried to highlight the major differences below since the branch has 63,000+ lines changed.
Behavior Removal/Changes
Validator Bonds
The existing lsm implementation made a couple of breaking changes to the staking module.
min_self_delegationfieldValidatorBondandUnbondValidatortransactionsThese changes introduced a new transaction type which specified a validator and a delegator indicating the intent to
mark a specific delegation as a validator bond. The amount for each validator which was marked specifically as
Validator Bonds was used along with the
validator_bond_factorfield which was added to the staking module'sParams.The combination of these things was used to determine a limit for how many liquid staking tokens a validator could
issue.
The PR removes the concept of
ValidatorBondandUnbondValidatoralong with the bond factor param. It removes thedeprecated label from the
min_self_delegationfield. The logic which uses the validator bonds in order to enforcelimits on tokenized shares has been preserved, but the calculations for limits now looks up self delegations from
the validator and uses that total as the validator bond amount.
Since the validator bond shares are stored in a new storage primitive specific to the lsm module,
LiquidValidator,this PR uses the staking hooks to update the value any time a delegation is updated--checking whether the delegation
was a validator self delegation and updating the value corresponding.
It's possible in a followup PR that we should remove these limits entirely. I advocate for setting this value to
ValidatorBondCapDisabledwhich removes this limit entirely when doing the upgrade.Liquid Staking Caps
The existing parameters limiting liquid stake issuance are
global_liquid_staking_capandvalidator_liquid_staking_cap. These limit the sum of all tokenized shares and the per-validator tokenized sharesrespectively.
This PR preserves those limits and calculations for tokenization of shares which occurs through the lsm module
itself. However, the existing implementation also altered the staking module itself to include delegations from ICAs.
The rationale behind this is that LST protocols are currently implemented as interchain applications and the
majority of their LSTs are issued on behalf of stake which is delegated via ICAs.
As we're no longer forking the staking module, it's not practical to calculate these values--the existing hooks
don't provide amounts. Also, the regulation of external protocols doesn't seem like something our module should be
trying to regulate so I've removed those calculations entirely and limited these parameters to lsm module backed shares.
Storage Primitives
Most of the existing storage primitives used in the upstream implementation are preserved.
TokenizeShareRecordsare still the basic way of recording tokenized delegations. The disabling and enabling of lsm functionality per
validator is preserved. And the
TokenizeShareRecordRewardhas been migrated from the distribution keeper forrewards tabulation.
The new primitive introduced for module storage is the
LiquidValidatorwhich is simply keyed on validator operatoraddress and stores the current bond shares (calculated via self delegations) and liquid shares issued for each
validator.
The upgrade code to accomplish these migrations and populate the module storage on upgrade is not included in this PR.
Keeper Overview
distribution.goThis contains all the code migrated from the upstream distribution keeper fork. It is used solely for withdrawing
rewards accumulated to a tokenize share record.
hooks.goThese hooks are all newly implemented. They mostly replace the parts of the code which were altering existing
staking, distribution, and slashing module code. Its purpose is to update validator bond shares and the
LiquidValidatorrecords which keep track of total liquid staked shares a validator has issued.liquid_stake.goDirectly ported from the upstream implementation. This is mostly convenience methods for interacting with and
implementing logic around liquid staking.
tokenize_share_record.goContains most of the logic for store updates and access.
Keeper Dependencies
In places where the previous implementation was using the forked Keepers directly I've had to replace them with
dependencies in the new module. This means the lsm module requires the
AccountKeeper,BankKeeper,StakingKeeper, andDistributionKeeperin order to work. You can see the required methods inexpected_keepers.go.In tests, I've added mocks generated via Mockery to replace these implementations.